Skip to content

Conversation

@HahaBill
Copy link
Contributor

@HahaBill HahaBill commented Jul 20, 2025

Related GitHub Issue

Closes: #5444

Description

Progressively migrating to new Gemini models and naming conventions. See in #5444.

Test Procedure

Frontend Testing

  1. Configure legacy model (e.g., gemini-1.5-pro-002) in provider settings
  2. Open provider settings - should display the migrated model (e.g., gemini-2.0-flash-001)
  3. Verify - No "unknown model" errors, correct model info shown

Backend Testing

  1. Set breakpoints in GeminiHandler.getModel()
  2. Debug and verify - Original legacy ID is mapped to current model ID
  3. Check consistency - this.options.apiModelId matches the frontend display

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Important

Update Gemini and Vertex models, add legacy model mapping, and enhance tests for model ID migration.

  • Model Updates:
    • Update geminiModels and vertexModels in gemini.ts and vertex.ts to include new models and remove outdated ones.
    • Add legacyGeminiModels and legacyVertexModels to handle legacy model IDs.
  • Model Mapping:
    • Add mapLegacyGeminiModel() in gemini.ts and useSelectedModel.ts to map legacy Gemini model IDs to current models.
    • Add mapLegacyVertexModel() in vertex.ts and useSelectedModel.ts to map legacy Vertex model IDs to current models.
  • Testing:
    • Add tests in gemini.spec.ts and vertex.spec.ts to verify legacy model ID mapping to current models.
    • Ensure tests cover scenarios for both Gemini and Vertex handlers, including error handling and model info retrieval.

This description was created by Ellipsis for 6dab2cd. You can customize this summary. It will automatically update as commits are pushed.

@HahaBill HahaBill requested review from cte, jr and mrubens as code owners July 20, 2025 22:27
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. enhancement New feature or request labels Jul 20, 2025
@HahaBill HahaBill changed the title I/update gemini and vertex models fix: Progressive migration to newer Gemini models and naming conventions Jul 20, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jul 20, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jul 21, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jul 21, 2025
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @HahaBill Thank you for your work on this, I left some minor points but I'll go ahead and approve the PR.

Also should we add a sunset date or version on which remove the migrations?

Let me know what you think!

}

if (modelId.startsWith("gemini-1.5-pro-")) {
return geminiDefaultModelId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed an inconsistency in the model mapping logic between Gemini and Vertex handlers. Here, gemini-1.5-pro-* maps to geminiDefaultModelId, while in the Vertex handler it explicitly maps to gemini-2.0-flash-001.

While both ultimately resolve to the same model (since geminiDefaultModelId is gemini-2.0-flash-001), would it be clearer to use the explicit model ID in both places for consistency? This would make the mapping logic more transparent and easier to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was planning to do that but realised that geminiDefaultModelId is a type of GeminiModelId, whereas Vertex models are type of VertexModelId. So there are different types.

(1) But gemini-2.0-flash-001 is currently hardcoded in the Vertex legacy mapping. I can actually create export const vertexDefaultGeminiModelId: VertexModelId = "gemini-2.0-flash-001" so then it's easier to maintain.

(2) Or we can do geminiDefaultModelId to be type of VertexModelId and GeminiModelId.

Which one of the two options do you think is better? I saw there is a PR that tries to separate Vertex from Gemini so maybe option (1) sounds better?

let id = modelId ? this.mapLegacyGeminiModel(modelId) : geminiDefaultModelId

if (modelId && modelId !== id) {
this.options.apiModelId = id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the decision to modify this.options.apiModelId directly here. Could this have unintended side effects if the options object is shared or referenced elsewhere?

Would it be safer to keep the original model ID unchanged and only use the mapped ID internally? This would preserve the original user configuration while still achieving the migration goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is because of doing an immediate automatic sync between the backend and frontend (webview-ui). This is why the mapping function on the backend and frontend is the same.

If users choose another models from the selection in the profile settings then the original model id would be overwritten by new (non-legacy) model id and that is fine. This if (modelId && modelId !== id) { will only happen if it's a legacy model.

We could just not do the mapping on the frontend-side but then users have to choose models for each Gemini profile. If not then the original (legacy) model ID might be still used (because that is still stored) and this could cause API errors because the original (legacy) model ID is not accessible by the Gemini API anymore.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 23, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jul 23, 2025
@HahaBill
Copy link
Contributor Author

Hey @HahaBill Thank you for your work on this, I left some minor points but I'll go ahead and approve the PR.

Also should we add a sunset date or version on which remove the migrations?

Let me know what you think!

I think the longer we keep the migration, the safer it is. Currently, it's summer so I expect people to not code that much :D I would say around mid-October sounds good.

@HahaBill
Copy link
Contributor Author

Screenshot 2025-07-24 at 10 10 37

I want to double check Gemini 2.0 variants because of user's comment from Discord on gemini-2.0-pro-exp-02-05 not being accessible

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to avoid duplicating the logic into this file? At least it seems the same as what's above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is exactly the same, I will try to unify them into one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I implemented the changes, now the mapping functions should be in the gemini.ts and vertex.ts in types folder. I did the test procedures which you can find in this PR's description (in debug mode) and everything seems to work.

The thing I noticed now that in old Gemini profiles, the "Save" is not disabled and you can either discard the changes or click on "Save".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be because when I change the model in useSelectedModel.ts which is tracked by SettingsViews.tsx.

Copy link
Contributor Author

@HahaBill HahaBill Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniel-lxs daniel-lxs moved this from PR [Needs Review] to PR [Changes Requested] in Roo Code Roadmap Jul 31, 2025
Comment on lines +222 to +253
const handleLegacyGeminiModelMapping = useCallback(
<K extends keyof ProviderSettings>(
field: K,
previousValue: ProviderSettings[K],
value: ProviderSettings[K],
provider: string | undefined,
updatedApiConfiguration: ProviderSettings,
): boolean => {
if (field !== "apiModelId" || typeof previousValue !== "string" || typeof value !== "string") {
return false
}

let isLegacyMapping = false
if (provider === "gemini") {
isLegacyMapping = mapLegacyGeminiModel(previousValue) === value
} else if (provider === "vertex") {
isLegacyMapping = mapLegacyVertexModel(previousValue) === value
}

if (isLegacyMapping && currentApiConfigName) {
vscode.postMessage({
type: "upsertApiConfiguration",
text: currentApiConfigName,
apiConfiguration: updatedApiConfiguration,
})
}

return isLegacyMapping
},
[currentApiConfigName],
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new change that I did, I realised that it wasn't persistently stored. We don't need the legacy mapping in gemini.ts anymore but we can leave it there just for the extra security.

Overall: we should have a clean ap state now with migrations being persistently stored with doing upsert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new change that I did, I realised that it wasn't persistently stored. We don't need the legacy mapping in gemini.ts anymore but we can leave it there just for the extra security.

Overall: we should have a clean app state now with migrations being persistently stored with doing upsert

@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to PR [Needs Prelim Review] in Roo Code Roadmap Aug 12, 2025
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @HahaBill for addressing the reviews, this looks good

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Aug 12, 2025
@daniel-lxs
Copy link
Member

I'll be closing this as stale for now, however feel free to let me know if we should get this PR ready again.

@daniel-lxs daniel-lxs closed this Sep 16, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Sep 16, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request lgtm This PR has been approved by a maintainer PR - Needs Review size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Updating Gemini and Vertex AI model's naming convention for gemini-2.5-pro-preview-{dates} to gemini-2.5-pro

4 participants